Skip to content

Bhamehta/code cleanup#2

Open
bmehta001 wants to merge 12 commits into
mainfrom
bhamehta/code-cleanup
Open

Bhamehta/code cleanup#2
bmehta001 wants to merge 12 commits into
mainfrom
bhamehta/code-cleanup

Conversation

@bmehta001
Copy link
Copy Markdown
Owner

@bmehta001 bmehta001 commented Mar 18, 2026 •
edited
Loading

Fixes runtime thread-safety, shutdown-safety, and response-lifetime issues that affect normal SDK operation. Split out from microsoft#1415 per reviewer request so runtime behavior changes stay separate from CI/build/test fixes.

Fix HTTP cancellation and callback tracking

HttpClient_Apple.mm

  • Scope cancellation to the current request’s m_dataTask instead of canceling every task on the shared static NSURLSession.
  • Fix the CancelAllRequests() wait loop so m_requests.empty() is checked while holding m_requestsMtx.

HttpClientManager.cpp

  • Fix the cancelAllRequests() wait loop so m_httpCallbacks.empty() is checked while holding m_httpCallbacksMtx.

Fix HTTP response lifetime on abort/network-failure paths

HttpResponseDecoder.cpp

  • Preserve ctx->httpResponse through requestAborted(ctx) and temporaryNetworkFailure(ctx) so downstream storage/statistics handlers can still read status and headers.
  • Avoid leaking aborted/network-failure responses by keeping ownership with EventsUploadContext, whose clear() path deletes the response.

HttpResponseDecoderTests.cpp

  • Add regression coverage that aborted and network-failure decode routes still receive the response object with result/status intact.

Fix WorkerThread shutdown safety

WorkerThread.cpp

  • Add an explicit shutdown gate so late Queue() calls are rejected once teardown starts.
  • Enqueue the shutdown sentinel only once under the queue lock.
  • Clean up pending queued/timer tasks only after a successful join(), and avoid cleanup after detach() because the worker may still be running.
  • Log join/detach failures instead of silently swallowing all exceptions.

Make TransmissionPolicyManager scheduling consistently mutex-guarded

TransmissionPolicyManager.cpp / .hpp

  • Guard m_isUploadScheduled, m_runningLatency, and m_scheduledUploadTime consistently with m_scheduledUploadMutex.
  • Avoid holding m_scheduledUploadMutex across potentially blocking cancellation during stop/shutdown.
  • Keep force/zero-delay no-wait cancellation under the scheduler mutex so a competing delayed schedule cannot suppress an immediate upload.
  • Use an explicit std::chrono::milliseconds value in the bandwidth-controller reschedule path.

TransmissionPolicyManagerTests.cpp

  • Add regression coverage for the force/zero-delay scheduling race.

Fix Logger static-destruction-order crash

Logger.cpp

  • Remove destructor logging from Logger::~Logger() because it can run after logging infrastructure has already been destroyed, causing crashes during static teardown.

Validation

  • HttpResponseDecoderTests.* and TransmissionPolicyManagerTests.ForceScheduleRetainsImmediateUploadWhenCancelBlocks passed.
  • Full host UnitTests passed: 485/485

@bmehta001 bmehta001 self-assigned this Mar 18, 2026
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch 20 times, most recently from 9a5a166 to 2feeb94 Compare March 19, 2026 05:40
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch 4 times, most recently from e303041 to 9a91839 Compare April 1, 2026 16:55
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch 5 times, most recently from b30c9b8 to 95502d5 Compare April 29, 2026 21:29
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch 3 times, most recently from 65fc991 to 0fe40ec Compare April 29, 2026 21:42
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch from b34147f to 0e8018c Compare May 15, 2026 03:06
@bmehta001
Update vendored zlib to 1.3.2 (microsoft#1443)
de38c4b 
* Update vendored zlib to 1.3.2

Refresh vendored zlib sources, headers, documentation, and minizip files to upstream v1.3.2 so the SDK tracks the current supported zlib release.

Preserve the SDK's symbol-prefix shim and legacy Windows project wiring, and update the zlib CMake shim so out-of-source builds no longer mutate zconf.h or define Windows macros on non-Windows hosts.

Files changed:

- cgmanifest.json

- zlib core sources, headers, docs, and minizip support files

- zlib/names.h and zlib/CMakeLists.txt repo-specific shims

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Move zlib MSBuild projects outside vendored tree

Keep the vendored zlib folder closer to the upstream 1.3.2 layout while preserving the legacy MSBuild project used by the Windows solution.

Files changed:

- Move vc14 zlib Visual Studio projects to third_party/Solutions/zlib/vc14

- Update solution, sample, and test ProjectReference paths

- Remove unused vc9/vc10/vc11 zlib project directories

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch from 17b72ca to 1d09c10 Compare May 22, 2026 07:35
bmehta001 and others added 11 commits May 22, 2026 12:33
@bmehta001
Remove stale MSBuild project entries (microsoft#1444)
0e2d728 
* Remove stale MSBuild project entries

Clean project and filter wiring that points at removed or generated files so Visual Studio metadata matches the files carried in the repo.

Files changed:

- Solutions/Clienttelemetry/Clienttelemetry.vcxitems

- examples/cpp project and filter files

- lib/pal and lib/tracing shared item files

- tests/googletest filter metadata

- third_party/Solutions/zlib/vc14/zlibvc.vcxproj

- tools/ports/mstelemetry/v142-build.patch

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Keep UWP package signing comments for documentation

The commented PackageCertificateKeyFile and PackageCertificateThumbprint
lines document how to enable Appx package signing if a developer provides
a TestApp.pfx. Restore them so the sample retains its re-enablement
instructions; only the unconditional None Include for TestApp.pfx
(which referenced a missing file) needed to go.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Restore conditional UTC filter entries in lib/pal

The matching desktop.vcxitems and universal.vcxitems still include the
UTC module sources via Condition="Exists(...)". Removing the parallel
entries from the .filters files would orphan those sources in Visual
Studio Solution Explorer when the lib/modules submodule is present.
Restore the conditional ItemGroups so solution organization stays
correct when the optional module exists.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Match SignalsFuncTests filter path to vcxproj entry

FuncTests.vcxproj uses $(ProjectDir)..\..\lib\modules\signals\tests\functests\SignalsFuncTests.cpp while the parallel filter entry was missing the $(ProjectDir) prefix, so when the signals submodule is present Visual Studio cannot associate the source file with the Source Files filter. Align the filter Include with the project Include.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Apply CMake best practices and remove stale references
13c6414 
- Modernize CMakeLists.txt: flatten nesting, deduplicate, use
  consistent quoting and variable patterns
- Remove stale header references from vcxproj and vcxitems files
- Simplify test CMakeLists.txt files
- Fix CMake conventions in packaging scripts

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Fix microsoft#1416 review follow-ups
7d9bbde 
Keep the before.targets toolset selection deterministic on newer
Visual Studio hosts, but only set the MIP props fallback when a
consumer has not already chosen a toolset. While addressing the
MSBuild review comments, also point the optional module test
conditions and source paths at the real lib/modules locations and
use CPACK_PACKAGE_FILE_NAME for the RPM status message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Fix microsoft#1416 Apple runtime regressions
13f66c1 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Make CurlHttpOperation own CA path
b47f5c0 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Restore nonfunctional CMake comments
028199c 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Remove runtime overlap with microsoft#1429
5c0ded8 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Potential fix for pull request finding
a262717 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@bmehta001
Quote functest configure_file paths
93502cc 
Quote the optional ECS test.json configure_file input and output paths so CMake handles source or build directories that contain spaces.

Files changed:

- tests/functests/CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Drop -ffast-math from Unix REL_FLAGS
704d29b 
PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing
-fno-finite-math-only from the three Unix REL_FLAGS branches and adding
-Wno-nan-infinity-disabled to silence the Clang diagnostic. That left
release builds using -ffast-math without preserving the NaN/Inf
semantics needed by nlohmann::json and SQLite paths.

Remove -ffast-math entirely from the GCC, AppleClang, and generic Clang
release flags rather than relying on -fno-finite-math-only to partially
undo it. This SDK is not floating-point compute-bound; its hot paths are
string, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes.

Avoiding -ffast-math:
- preserves std::isnan/std::isinf behavior for JSON and storage code,
- avoids compiler/runtime fast-math side effects such as x86 GCC's
  crtfastmath.o changing MXCSR FTZ/DAZ behavior process-wide, and
- aligns with SQLite's guidance to avoid fast-math.

This subsumes microsoft#1415's partial mitigation and aligns with microsoft#1392's intent,
extended to GCC because GCC fast-math has the same broad assumptions and
runtime side effects. Also remove -Wno-nan-infinity-disabled because the
warning should not be suppressed once the cause is gone.

Validation:
- CMake Release build of UnitTests on macOS arm64 (AppleClang 21).
- UnitTests passed on macOS arm64.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Restore Clang warning comment clarity
1cfe326 
Avoid carrying a confusing note about warning flags that are not used, and keep CMakeLists.txt focused on active compiler settings.

Files changed:

- CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 force-pushed the bhamehta/code-cleanup branch from 1d09c10 to 1cfe326 Compare May 22, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bmehta001 bmehta001

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bmehta001